Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose decision support values table in ehrQL #2348

Merged
merged 4 commits into from
Jan 10, 2025

Conversation

rw251
Copy link
Contributor

@rw251 rw251 commented Jan 9, 2025

Closes #2264

Some thoughts:

  • The AlgorithmType column is used to join the tables, but I've also exposed it to users. Given it's an internal TPP thing I'm not sure that's necessary - but then again to pick a particular algorithm you either need to search by algorithm_type or algorithm_description - both of which are defined by TPP so perhaps not much difference
  • Are there any checks missing? It feels a bit uncomfortable exposing a new table but not being able to actually test the code on the actual database. Especially because the way we identify the EFI is by searching for a hard-coded string definition and version number.
  • For the EFI, I've copied code from @evansd which looks for descriptions matching "UK Electronic Frailty Index (eFI)". I just want to double check this is the correct string. In cohort extractor the search was actually done by looking for an AlgorithmType of 1, so perhaps we should do that.
  • Do we have any way of checking the possible values in the Type, Description and Version columns. I think at the moment it is just v1 of the EFI, but it would be good to know if TPP change anything or add new ones.

Copy link
Contributor

@evansd evansd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me, thanks.

The questions you raise are all good ones, and I think there are some reasonable answers which I'd like to go into but don't have time before the end of the day. I just thought it was worth at least approving the PR now and then we can discuss tomorrow.

@rw251 rw251 merged commit 32f477b into main Jan 10, 2025
9 checks passed
@rw251 rw251 deleted the rw/decision-support-value-table branch January 10, 2025 08:09
@evansd
Copy link
Contributor

evansd commented Jan 10, 2025

On the question of filtering on AlgorithmType vs AlgorithmDescription, the way I see it is this. Neither field has been formally specified to us as having stable values so there are, in principle, two failures modes:

  1. we try to return EFI results but something has changed and we don't return any results;
  2. we try to return EFI results but something has changed and we return the wrong results.

Of the two, it seems to me that 1 is far more preferable. It's likely to be noticed immediately by the researcher, and even if it isn't missing values are likely to be overall less harmful than incorrect values. And filtering by the description is, I think, far more likely to fail in first way whereas filtering by the type number runs a greater risk of failing in the second way. That is, it's very unlikely that the string "UK Electronic Frailty Index (eFI)" will refer to anything other than the thing we want. But it's not inconceivable that "Algorithm Type 1" might do.

In fact, I might be minded to not expose AlgorithmType at all to users just to avoid the risk that they start relying on it.

On the question of what the values are, we have now included these in the TPP database schema reports we generate. However, I don't think we've yet re-run the report. And we haven't included it in the set of things we automatically fetch and include in the ehrQL repo. We should do that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose DecsionSupportValues table in ehrQL
2 participants